Skip to content

Comments

Fix and improve tests#234

Merged
TobiGr merged 11 commits intodevfrom
fix/YT_search_test
Jan 20, 2020
Merged

Fix and improve tests#234
TobiGr merged 11 commits intodevfrom
fix/YT_search_test

Conversation

@TobiGr
Copy link
Contributor

@TobiGr TobiGr commented Jan 8, 2020

Another attempt to fix some of the failing tests.

The testViewCount() test is quite illogical because

  • the test name does not fit to what it does,
  • we cannot get the subscriber count of a channel when using the search,
  • it is the only method in the file. I guess someone forgot to delete it after refactoring or whatsoever.

I guess the test's purpose was to check the channel's stream count and therefore modified and moved it.

Sometimes tests are good to find bugs :) I added a null check to LinkHandlerFactory.fromUrl(String url, String baseUrl).

These two tests drive me crazy:

@Test
 public void testGetSubtitlesListDefault() throws IOException, ExtractionException {
    // Video (/view?v=YQHsXMglC9A) set in the setUp() method has no captions => null
    assertFalse(extractor.getSubtitlesDefault().isEmpty());
}

@Test
public void testGetSubtitlesList() throws IOException, ExtractionException {
    // Video (/view?v=YQHsXMglC9A) set in the setUp() method has no captions => null
    assertFalse(extractor.getSubtitles(MediaFormat.TTML).isEmpty());
}

Why is the subtitle list expected to be NOT empty? I think the only logical result is here to have an empty list.

@TobiGr TobiGr added bug Issue is related to a bug youtube service, https://www.youtube.com/ labels Jan 8, 2020
@TobiGr
Copy link
Contributor Author

TobiGr commented Jan 13, 2020

I thought again about the subtitle tests, but did not find a reason to use assertFalse.

@TobiGr TobiGr requested a review from Stypox January 13, 2020 15:24
@TobiGr
Copy link
Contributor Author

TobiGr commented Jan 13, 2020

Strange. The subtitle tests failed locally when using assertFalse, the Travis test fail when using assertTrue. Why?

Stypox
Stypox requested changes Jan 20, 2020
View reviewed changes
Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I understood your concern about captions. The video has captions, and currently the extractor is able to extract them, so assertFalse(... .isEmpty()) is correct.

@Stypox
Copy link
Member

Stypox commented Jan 20, 2020

While you are at it, could you also fix these failing tests?

org.schabi.newpipe.extractor.services.youtube.YoutubeStreamLinkHandlerFactoryTest > getIdfromYt FAILED
    org.schabi.newpipe.extractor.exceptions.ParsingException: Malformed url: vnd.youtube://www.youtube.com/watch?v=jZViOEv90dI
        at org.schabi.newpipe.extractor.utils.Utils.getBaseUrl(Utils.java:184)
        at org.schabi.newpipe.extractor.linkhandler.LinkHandlerFactory.fromUrl(LinkHandlerFactory.java:47)
        at org.schabi.newpipe.extractor.services.youtube.YoutubeStreamLinkHandlerFactoryTest.getIdfromYt(YoutubeStreamLinkHandlerFactoryTest.java:80)
        Caused by:
        java.net.MalformedURLException: unknown protocol: vnd.youtube
            at java.base/java.net.URL.<init>(URL.java:634)
            at java.base/java.net.URL.<init>(URL.java:523)
            at java.base/java.net.URL.<init>(URL.java:470)
            at org.schabi.newpipe.extractor.utils.Utils.stringToURL(Utils.java:145)
            at org.schabi.newpipe.extractor.utils.Utils.getBaseUrl(Utils.java:182)
            ... 2 more

and

org.schabi.newpipe.extractor.services.youtube.YoutubeChannelExtractorTest$CaptainDisillusion > testMoreRelatedItems FAILED
    java.lang.IllegalArgumentException: String input must not be null
        at org.jsoup.helper.Validate.notNull(Validate.java:26)
        at org.jsoup.parser.TreeBuilder.initialiseParse(TreeBuilder.java:26)
        at org.jsoup.parser.TreeBuilder.parse(TreeBuilder.java:42)
        at org.jsoup.parser.HtmlTreeBuilder.parse(HtmlTreeBuilder.java:52)
        at org.jsoup.parser.Parser.parse(Parser.java:89)
        at org.jsoup.Jsoup.parse(Jsoup.java:31)
        at org.schabi.newpipe.extractor.services.youtube.extractors.YoutubeChannelExtractor.getPage(YoutubeChannelExtractor.java:193)
        at org.schabi.newpipe.extractor.services.DefaultTests.defaultTestMoreItems(DefaultTests.java:55)
        at org.schabi.newpipe.extractor.services.youtube.YoutubeChannelExtractorTest$CaptainDisillusion.testMoreRelatedItems(YoutubeChannelExtractorTest.java:361)

@Stypox Stypox mentioned this pull request Jan 20, 2020
Closed
@TobiGr TobiGr force-pushed the fix/YT_search_test branch from 3e32222 to 2308b07 Compare January 20, 2020 21:12
@TobiGr
Copy link
Contributor Author

TobiGr commented Jan 20, 2020

Regarding the first one: there are two links which are only extracted from youtube websites. That's why it should be save to set the service to youtube / pass the baseUrl.

Hooray! Some other tests are failing now. But not at my local machine 🎉

Stypox
Stypox approved these changes Jan 20, 2020
View reviewed changes
Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh ok, if tests succeed at your local machine the failing tests on tarvis can probably be ignored since they are most probably caused by service overloading.

This is ready (I would merge it but now I am from mobile), thank you!

@TobiGr
Copy link
Contributor Author

TobiGr commented Jan 20, 2020

Reran the tests and they finished without failing 🎉

@TobiGr TobiGr merged commit 82eff77 into dev Jan 20, 2020
@TobiGr TobiGr deleted the fix/YT_search_test branch January 20, 2020 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Stypox Stypox Stypox approved these changes

Assignees

No one assigned

Labels

bug Issue is related to a bug youtube service, https://www.youtube.com/

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@TobiGr @Stypox